Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add bookmark popover in SwiftUI #2036

Merged
merged 21 commits into from
Jan 16, 2024
Merged

Conversation

mallexxx
Copy link
Collaborator

@mallexxx mallexxx commented Jan 9, 2024

Task/Issue URL: https://app.asana.com/0/276630244458377/1206189080947121/f

Description:

  • Bookmark Added popover and Add Folder popover contents refactored in SwiftUI
    • Generalised MenuBuilder to generic ArrayBuilder
    • Updated NSPopupButtonView to allow dynamic SwiftUI content

Steps to test this PR:

  1. Add a bookmark using Cmd+D or button in the Address Bar
  2. Validate Bookmark Added popover is shown.
  3. Click Remove - validate the bookmark is removed
  4. Add bookmark again and change folder at least twice - validate selected folder stays in the picker and is changed for the bookmark in Bookmark Manager. Reopen Bookmark Popover - validate bookmark folder is selected
  5. Click Add Folder, validate initially selected folder matches current bookmark folder.
  6. Select another folder; click Cancel - validate UI is returned to the Add Bookmark state and bookmark folder is not changed
  7. Click Add Folder, select another folder, enter something in the Folder Title field, click Add - validate bookmark UI is shown and the new folder is selected
  8. Validate Add/Remove from favorites button adds/removes from favorites

Internal references:

Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation

@mallexxx mallexxx requested a review from ayoy January 9, 2024 13:04
@mallexxx mallexxx changed the base branch from main to alex/add-folder-modal-swiftui January 9, 2024 13:05
@mallexxx mallexxx changed the base branch from alex/add-folder-modal-swiftui to alex/add-bookmark-modal-swiftui January 9, 2024 13:05
@mallexxx mallexxx changed the base branch from alex/add-bookmark-modal-swiftui to main January 9, 2024 13:06
Copy link
Contributor

github-actions bot commented Jan 9, 2024

Warnings
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS against 034c1ac

Copy link
Collaborator

@ayoy ayoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks and works great! Thanks @mallexxx!

didSet {
if oldValue?.id != selectedFolder?.id {
bookmarkManager.add(bookmark: bookmark, to: selectedFolder) { _ in
// this is an invalid callback fired before bookmarks finish reloading
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤯

func updateNSView(_ button: NSPopUpButton, context: NSViewRepresentableContext<NSPopUpButtonView>) {
if let content {
let newContent = content()
let diff = newContent.difference(from: context.coordinator.content) { $0 == $1 }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love this

@ayoy ayoy assigned mallexxx and unassigned ayoy Jan 16, 2024
@ayoy
Copy link
Collaborator

ayoy commented Jan 16, 2024

@mallexxx I think I'm getting a diff in Localizable.xcstrings again. Could you make sure it's up to date before merging? Thanks!

diff --git a/DuckDuckGo/Localizable.xcstrings b/DuckDuckGo/Localizable.xcstrings
index a7ba1fe5d..ffd4e14e9 100644
--- a/DuckDuckGo/Localizable.xcstrings
+++ b/DuckDuckGo/Localizable.xcstrings
@@ -4330,7 +4330,7 @@
       }
     },
     "Name:" : {
-      "comment" : "New bookmark folder dialog folder name field heading\nAdd Folder popover: folder name text field title"
+      "comment" : "Add Folder popover: folder name text field title\nNew bookmark folder dialog folder name field heading"
     },
     "navigate.back" : {
       "comment" : "Context menu item",

@mallexxx mallexxx merged commit 2c05719 into main Jan 16, 2024
14 checks passed
@mallexxx mallexxx deleted the alex/add-bookmark-popover-swiftui branch January 16, 2024 16:24
samsymons added a commit that referenced this pull request Jan 17, 2024
# By Dominik Kapusta (16) and others
# Via Dominik Kapusta (2) and others
* main: (67 commits)
  Increase polling interval for captcha solver (#2071)
  Add a link to help page in location permission popup (#2017)
  Add bookmark popover in SwiftUI (#2036)
  Better detection of AMP pages (#2001)
  UI adjustments for improved VPN user control (#2043)
  Don't override drop operation if source view is webView (#2058)
  Bump version to 1.71.0 (104)
  Set marketing version to 1.71.0
  Update embedded files
  VPN Geoswitching - look and feel tweaks + Pixels (#2045)
  fix Bookmarks Manager empty state constraint (#2034)
  Add Folder modal in SwiftUI (#2033)
  Fix serverlocation in connection notification (#2053)
  Use GRDB.swift 2.23.0 (upstream 6.32.0, SQLCipher 4.5.5) (#2021)
  Bump version to 1.70.0 (103)
  Update embedded files
  macOS: Fix issue with not being able to add favorites for existing bookmarks in the latest 1.70.0 internal (#2059)
  Bump version to 1.70.0 (102)
  Update embedded files
  Remove observer from DataBrokerProtectionViewController (#2057)
  ...

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
#	LocalPackages/BuildToolPlugins/Plugins/InputFilesChecker/InputFilesChecker.swift
samsymons added a commit that referenced this pull request Jan 18, 2024
* main:
  Bump BrowserServicesKit Version (#2063)
  Bump autoconsent to 9.1.0 (#2067)
  Fix casing in some copy items in Preferences and Main Menu (#2070)
  Fix vertical alignment of privacy feed shield icon (#2069)
  Fix giving focus to webView when it's loaded in BrowserTabViewController (#2073)
  Increase polling interval for captcha solver (#2071)
  Add a link to help page in location permission popup (#2017)
  Add bookmark popover in SwiftUI (#2036)
  Better detection of AMP pages (#2001)
  UI adjustments for improved VPN user control (#2043)
  Don't override drop operation if source view is webView (#2058)
  Bump version to 1.71.0 (104)
  Set marketing version to 1.71.0
  Update embedded files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants